Conversation
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ptimize clang-tidy run
There was a problem hiding this comment.
Cpp-linter Review
Used clang-tidy v20.1.2
Click here for the full clang-tidy patch
diff --git a/src/audio_stream.cpp b/src/audio_stream.cpp
index ef9263c..6ec0150 100644
--- a/src/audio_stream.cpp
+++ b/src/audio_stream.cpp
@@ -216 +216 @@ void AudioStream::onFfiEvent(const FfiEvent &event) {
- AudioFrameEvent ev{std::move(frame)};
+ AudioFrameEvent ev{frame};
diff --git a/src/rpc_error.cpp b/src/rpc_error.cpp
index 1e521cd..96848ee 100644
--- a/src/rpc_error.cpp
+++ b/src/rpc_error.cpp
@@ -52 +52 @@ RpcError RpcError::fromProto(const proto::RpcError &err) {
- return RpcError(err.code(), err.message(), err.data());
+ return {err.code(), err.message(), err.data()};
@@ -57 +57 @@ RpcError RpcError::builtIn(ErrorCode code, const std::string &data) {
- return RpcError(code, msg ? std::string(msg) : std::string{}, data);
+ return {code, msg ? std::string(msg) : std::string{}, data};
diff --git a/src/room.cpp b/src/room.cpp
index f4ffd74..e6375d3 100644
--- a/src/room.cpp
+++ b/src/room.cpp
@@ -300 +300 @@ void Room::setOnVideoFrameCallback(const std::string &participant_identity,
- participant_identity, source, std::move(callback), std::move(opts));
+ participant_identity, source, std::move(callback), opts);
@@ -310 +310 @@ void Room::setOnVideoFrameCallback(const std::string &participant_identity,
- participant_identity, track_name, std::move(callback), std::move(opts));
+ participant_identity, track_name, std::move(callback), opts);
diff --git a/src/video_frame.cpp b/src/video_frame.cpp
index 870dfee..63a30b3 100644
--- a/src/video_frame.cpp
+++ b/src/video_frame.cpp
@@ -298 +298 @@ VideoFrame VideoFrame::create(int width, int height, VideoBufferType type) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
@@ -306 +306 @@ std::vector<VideoPlaneInfo> VideoFrame::planeInfos() const {
- uintptr_t base = reinterpret_cast<uintptr_t>(data_.data());
+ auto base = reinterpret_cast<uintptr_t>(data_.data());
@@ -318 +318 @@ VideoFrame VideoFrame::convert(VideoBufferType dst, bool flip_y) const {
- return VideoFrame(width_, height_, type_, std::move(buf));
+ return {width_, height_, type_, std::move(buf)};
@@ -376 +376 @@ VideoFrame VideoFrame::fromOwnedInfo(const proto::OwnedVideoBuffer &owned) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
diff --git a/src/data_stream.cpp b/src/data_stream.cpp
index 70d871a..4117e0d 100644
--- a/src/data_stream.cpp
+++ b/src/data_stream.cpp
@@ -22,0 +23 @@
+#include <utility>
@@ -107 +108 @@ void fillBaseInfo(BaseStreamInfo &dst, const std::string &stream_id,
-TextStreamReader::TextStreamReader(const TextStreamInfo &info) : info_(info) {}
+TextStreamReader::TextStreamReader(TextStreamInfo info) : info_(std::move(info)) {}
@@ -151 +152 @@ std::string TextStreamReader::readAll() {
-ByteStreamReader::ByteStreamReader(const ByteStreamInfo &info) : info_(info) {}
+ByteStreamReader::ByteStreamReader(ByteStreamInfo info) : info_(std::move(info)) {}
@@ -192 +193 @@ BaseStreamWriter::BaseStreamWriter(
- LocalParticipant &local_participant, const std::string &topic,
+ LocalParticipant &local_participant, std::string topic,
@@ -195 +196 @@ BaseStreamWriter::BaseStreamWriter(
- const std::string &mime_type,
+ std::string mime_type,
@@ -197 +198 @@ BaseStreamWriter::BaseStreamWriter(
- const std::string &sender_identity)
+ std::string sender_identity)
@@ -200 +201 @@ BaseStreamWriter::BaseStreamWriter(
- mime_type_(mime_type), topic_(topic),
+ mime_type_(std::move(mime_type)), topic_(std::move(topic)),
@@ -206 +207 @@ BaseStreamWriter::BaseStreamWriter(
- sender_identity_(sender_identity) {
+ sender_identity_(std::move(sender_identity)) {
diff --git a/src/subscription_thread_dispatcher.cpp b/src/subscription_thread_dispatcher.cpp
index ee68365..7c548e8 100644
--- a/src/subscription_thread_dispatcher.cpp
+++ b/src/subscription_thread_dispatcher.cpp
@@ -90 +90 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback(
- RegisteredVideoCallback{std::move(callback), std::move(opts)};
+ RegisteredVideoCallback{std::move(callback), opts};
@@ -105 +105 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback(
- RegisteredVideoCallback{std::move(callback), std::move(opts)};
+ RegisteredVideoCallback{std::move(callback), opts};
@@ -478 +478 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked(
- AudioFrameCallback cb, const AudioStream::Options &opts) {
+ const AudioFrameCallback& cb, const AudioStream::Options &opts) {
@@ -501 +501 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked(
- auto stream_copy = stream;
+ const auto& stream_copy = stream;
@@ -529 +529 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked(
- VideoFrameCallback cb, const VideoStream::Options &opts) {
+ const VideoFrameCallback& cb, const VideoStream::Options &opts) {
@@ -552 +552 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked(
- auto stream_copy = stream;
+ const auto& stream_copy = stream;
@@ -623 +623 @@ std::thread SubscriptionThreadDispatcher::startDataReaderLocked(
- const std::shared_ptr<RemoteDataTrack> &track, DataFrameCallback cb) {
+ const std::shared_ptr<RemoteDataTrack> &track, const DataFrameCallback& cb) {
diff --git a/src/audio_frame.cpp b/src/audio_frame.cpp
index 23ff01e..5d58f6e 100644
--- a/src/audio_frame.cpp
+++ b/src/audio_frame.cpp
@@ -57,2 +57,2 @@ AudioFrame AudioFrame::create(int sample_rate, int num_channels,
- return AudioFrame(std::move(data), sample_rate, num_channels,
- samples_per_channel);
+ return {std::move(data), sample_rate, num_channels,
+ samples_per_channel};
@@ -72 +72 @@ AudioFrame::fromOwnedInfo(const proto::OwnedAudioFrameBuffer &owned) {
- const std::int16_t *ptr =
+ const auto *ptr =
@@ -91,2 +91,2 @@ AudioFrame::fromOwnedInfo(const proto::OwnedAudioFrameBuffer &owned) {
- return AudioFrame(std::move(data), sample_rate, num_channels,
- samples_per_channel);
+ return {std::move(data), sample_rate, num_channels,
+ samples_per_channel};
Have any feedback or feature suggestions? Share it here.
There was a problem hiding this comment.
Cpp-linter Review
Used clang-tidy v20.1.2
Click here for the full clang-tidy patch
diff --git a/src/audio_frame.cpp b/src/audio_frame.cpp
index 23ff01e..5d58f6e 100644
--- a/src/audio_frame.cpp
+++ b/src/audio_frame.cpp
@@ -57,2 +57,2 @@ AudioFrame AudioFrame::create(int sample_rate, int num_channels,
- return AudioFrame(std::move(data), sample_rate, num_channels,
- samples_per_channel);
+ return {std::move(data), sample_rate, num_channels,
+ samples_per_channel};
@@ -72 +72 @@ AudioFrame::fromOwnedInfo(const proto::OwnedAudioFrameBuffer &owned) {
- const std::int16_t *ptr =
+ const auto *ptr =
@@ -91,2 +91,2 @@ AudioFrame::fromOwnedInfo(const proto::OwnedAudioFrameBuffer &owned) {
- return AudioFrame(std::move(data), sample_rate, num_channels,
- samples_per_channel);
+ return {std::move(data), sample_rate, num_channels,
+ samples_per_channel};
diff --git a/src/data_stream.cpp b/src/data_stream.cpp
index 70d871a..4117e0d 100644
--- a/src/data_stream.cpp
+++ b/src/data_stream.cpp
@@ -22,0 +23 @@
+#include <utility>
@@ -107 +108 @@ void fillBaseInfo(BaseStreamInfo &dst, const std::string &stream_id,
-TextStreamReader::TextStreamReader(const TextStreamInfo &info) : info_(info) {}
+TextStreamReader::TextStreamReader(TextStreamInfo info) : info_(std::move(info)) {}
@@ -151 +152 @@ std::string TextStreamReader::readAll() {
-ByteStreamReader::ByteStreamReader(const ByteStreamInfo &info) : info_(info) {}
+ByteStreamReader::ByteStreamReader(ByteStreamInfo info) : info_(std::move(info)) {}
@@ -192 +193 @@ BaseStreamWriter::BaseStreamWriter(
- LocalParticipant &local_participant, const std::string &topic,
+ LocalParticipant &local_participant, std::string topic,
@@ -195 +196 @@ BaseStreamWriter::BaseStreamWriter(
- const std::string &mime_type,
+ std::string mime_type,
@@ -197 +198 @@ BaseStreamWriter::BaseStreamWriter(
- const std::string &sender_identity)
+ std::string sender_identity)
@@ -200 +201 @@ BaseStreamWriter::BaseStreamWriter(
- mime_type_(mime_type), topic_(topic),
+ mime_type_(std::move(mime_type)), topic_(std::move(topic)),
@@ -206 +207 @@ BaseStreamWriter::BaseStreamWriter(
- sender_identity_(sender_identity) {
+ sender_identity_(std::move(sender_identity)) {
diff --git a/src/video_frame.cpp b/src/video_frame.cpp
index 870dfee..63a30b3 100644
--- a/src/video_frame.cpp
+++ b/src/video_frame.cpp
@@ -298 +298 @@ VideoFrame VideoFrame::create(int width, int height, VideoBufferType type) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
@@ -306 +306 @@ std::vector<VideoPlaneInfo> VideoFrame::planeInfos() const {
- uintptr_t base = reinterpret_cast<uintptr_t>(data_.data());
+ auto base = reinterpret_cast<uintptr_t>(data_.data());
@@ -318 +318 @@ VideoFrame VideoFrame::convert(VideoBufferType dst, bool flip_y) const {
- return VideoFrame(width_, height_, type_, std::move(buf));
+ return {width_, height_, type_, std::move(buf)};
@@ -376 +376 @@ VideoFrame VideoFrame::fromOwnedInfo(const proto::OwnedVideoBuffer &owned) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
diff --git a/src/subscription_thread_dispatcher.cpp b/src/subscription_thread_dispatcher.cpp
index ee68365..7c548e8 100644
--- a/src/subscription_thread_dispatcher.cpp
+++ b/src/subscription_thread_dispatcher.cpp
@@ -90 +90 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback(
- RegisteredVideoCallback{std::move(callback), std::move(opts)};
+ RegisteredVideoCallback{std::move(callback), opts};
@@ -105 +105 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback(
- RegisteredVideoCallback{std::move(callback), std::move(opts)};
+ RegisteredVideoCallback{std::move(callback), opts};
@@ -478 +478 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked(
- AudioFrameCallback cb, const AudioStream::Options &opts) {
+ const AudioFrameCallback& cb, const AudioStream::Options &opts) {
@@ -501 +501 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked(
- auto stream_copy = stream;
+ const auto& stream_copy = stream;
@@ -529 +529 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked(
- VideoFrameCallback cb, const VideoStream::Options &opts) {
+ const VideoFrameCallback& cb, const VideoStream::Options &opts) {
@@ -552 +552 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked(
- auto stream_copy = stream;
+ const auto& stream_copy = stream;
@@ -623 +623 @@ std::thread SubscriptionThreadDispatcher::startDataReaderLocked(
- const std::shared_ptr<RemoteDataTrack> &track, DataFrameCallback cb) {
+ const std::shared_ptr<RemoteDataTrack> &track, const DataFrameCallback& cb) {
Have any feedback or feature suggestions? Share it here.
There was a problem hiding this comment.
Cpp-linter Review
Used clang-tidy v20.1.2
Click here for the full clang-tidy patch
diff --git a/src/subscription_thread_dispatcher.cpp b/src/subscription_thread_dispatcher.cpp
index ee68365..7c548e8 100644
--- a/src/subscription_thread_dispatcher.cpp
+++ b/src/subscription_thread_dispatcher.cpp
@@ -90 +90 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback(
- RegisteredVideoCallback{std::move(callback), std::move(opts)};
+ RegisteredVideoCallback{std::move(callback), opts};
@@ -105 +105 @@ void SubscriptionThreadDispatcher::setOnVideoFrameCallback(
- RegisteredVideoCallback{std::move(callback), std::move(opts)};
+ RegisteredVideoCallback{std::move(callback), opts};
@@ -478 +478 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked(
- AudioFrameCallback cb, const AudioStream::Options &opts) {
+ const AudioFrameCallback& cb, const AudioStream::Options &opts) {
@@ -501 +501 @@ std::thread SubscriptionThreadDispatcher::startAudioReaderLocked(
- auto stream_copy = stream;
+ const auto& stream_copy = stream;
@@ -529 +529 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked(
- VideoFrameCallback cb, const VideoStream::Options &opts) {
+ const VideoFrameCallback& cb, const VideoStream::Options &opts) {
@@ -552 +552 @@ std::thread SubscriptionThreadDispatcher::startVideoReaderLocked(
- auto stream_copy = stream;
+ const auto& stream_copy = stream;
@@ -623 +623 @@ std::thread SubscriptionThreadDispatcher::startDataReaderLocked(
- const std::shared_ptr<RemoteDataTrack> &track, DataFrameCallback cb) {
+ const std::shared_ptr<RemoteDataTrack> &track, const DataFrameCallback& cb) {
diff --git a/src/video_frame.cpp b/src/video_frame.cpp
index 870dfee..63a30b3 100644
--- a/src/video_frame.cpp
+++ b/src/video_frame.cpp
@@ -298 +298 @@ VideoFrame VideoFrame::create(int width, int height, VideoBufferType type) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
@@ -306 +306 @@ std::vector<VideoPlaneInfo> VideoFrame::planeInfos() const {
- uintptr_t base = reinterpret_cast<uintptr_t>(data_.data());
+ auto base = reinterpret_cast<uintptr_t>(data_.data());
@@ -318 +318 @@ VideoFrame VideoFrame::convert(VideoBufferType dst, bool flip_y) const {
- return VideoFrame(width_, height_, type_, std::move(buf));
+ return {width_, height_, type_, std::move(buf)};
@@ -376 +376 @@ VideoFrame VideoFrame::fromOwnedInfo(const proto::OwnedVideoBuffer &owned) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
Have any feedback or feature suggestions? Share it here.
There was a problem hiding this comment.
Cpp-linter Review
Used clang-tidy v20.1.2
Click here for the full clang-tidy patch
diff --git a/src/video_frame.cpp b/src/video_frame.cpp
index 870dfee..63a30b3 100644
--- a/src/video_frame.cpp
+++ b/src/video_frame.cpp
@@ -298 +298 @@ VideoFrame VideoFrame::create(int width, int height, VideoBufferType type) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
@@ -306 +306 @@ std::vector<VideoPlaneInfo> VideoFrame::planeInfos() const {
- uintptr_t base = reinterpret_cast<uintptr_t>(data_.data());
+ auto base = reinterpret_cast<uintptr_t>(data_.data());
@@ -318 +318 @@ VideoFrame VideoFrame::convert(VideoBufferType dst, bool flip_y) const {
- return VideoFrame(width_, height_, type_, std::move(buf));
+ return {width_, height_, type_, std::move(buf)};
@@ -376 +376 @@ VideoFrame VideoFrame::fromOwnedInfo(const proto::OwnedVideoBuffer &owned) {
- return VideoFrame(width, height, type, std::move(buffer));
+ return {width, height, type, std::move(buffer)};
diff --git a/src/trace/event_tracer.cpp b/src/trace/event_tracer.cpp
index 8809993..3340f00 100644
--- a/src/trace/event_tracer.cpp
+++ b/src/trace/event_tracer.cpp
@@ -40,2 +40,2 @@
-namespace livekit {
-namespace trace {
+
+namespace livekit::trace {
@@ -179,3 +179,3 @@ std::string FormatEventJson(const TraceEventData &event, uint64_t start_time) {
- oss << "\"ph\":\"" << event.phase << "\",";
- oss << "\"cat\":\"" << JsonEscape(event.category) << "\",";
- oss << "\"name\":\"" << JsonEscape(event.name) << "\",";
+ oss << R"("ph":")" << event.phase << "\",";
+ oss << R"("cat":")" << JsonEscape(event.category) << "\",";
+ oss << R"("name":")" << JsonEscape(event.name) << "\",";
@@ -187 +187 @@ std::string FormatEventJson(const TraceEventData &event, uint64_t start_time) {
- oss << ",\"id\":\"0x" << std::hex << event.id << std::dec << "\"";
+ oss << R"(,"id":"0x)" << std::hex << event.id << std::dec << "\"";
@@ -430 +430 @@ void StopTracing() {
- g_trace_file << "],\"displayTimeUnit\":\"ms\"}";
+ g_trace_file << R"(],"displayTimeUnit":"ms"})";
@@ -444,2 +444,2 @@ bool IsTracingEnabled() {
-} // namespace trace
-} // namespace livekit
+} // namespace livekit::trace
+
Have any feedback or feature suggestions? Share it here.
There was a problem hiding this comment.
Cpp-linter Review
Used clang-tidy v20.1.2
Click here for the full clang-tidy patch
diff --git a/src/trace/event_tracer.cpp b/src/trace/event_tracer.cpp
index 8809993..3340f00 100644
--- a/src/trace/event_tracer.cpp
+++ b/src/trace/event_tracer.cpp
@@ -40,2 +40,2 @@
-namespace livekit {
-namespace trace {
+
+namespace livekit::trace {
@@ -179,3 +179,3 @@ std::string FormatEventJson(const TraceEventData &event, uint64_t start_time) {
- oss << "\"ph\":\"" << event.phase << "\",";
- oss << "\"cat\":\"" << JsonEscape(event.category) << "\",";
- oss << "\"name\":\"" << JsonEscape(event.name) << "\",";
+ oss << R"("ph":")" << event.phase << "\",";
+ oss << R"("cat":")" << JsonEscape(event.category) << "\",";
+ oss << R"("name":")" << JsonEscape(event.name) << "\",";
@@ -187 +187 @@ std::string FormatEventJson(const TraceEventData &event, uint64_t start_time) {
- oss << ",\"id\":\"0x" << std::hex << event.id << std::dec << "\"";
+ oss << R"(,"id":"0x)" << std::hex << event.id << std::dec << "\"";
@@ -430 +430 @@ void StopTracing() {
- g_trace_file << "],\"displayTimeUnit\":\"ms\"}";
+ g_trace_file << R"(],"displayTimeUnit":"ms"})";
@@ -444,2 +444,2 @@ bool IsTracingEnabled() {
-} // namespace trace
-} // namespace livekit
+} // namespace livekit::trace
+
Have any feedback or feature suggestions? Share it here.
| case proto::KIND_LOSSY: | ||
| return DataPacketKind::Lossy; | ||
| case proto::KIND_RELIABLE: | ||
| return DataPacketKind::Reliable; |
There was a problem hiding this comment.
can we remove the default instead ? that will allow us to catch the problem in compilation
|
|
||
| // Listener id registered on FfiClient | ||
| std::int64_t listener_id_{0}; | ||
| std::int32_t listener_id_{0}; |
There was a problem hiding this comment.
Is this change intentional ?
Didn't you mention that you would like to change the listener id to int64_t ?
There was a problem hiding this comment.
This was intentional: our options are to move this to int64_t:
client-sdk-cpp/src/ffi_client.h
Line 68 in e69c646
Or adjust the other listener ID types internally.
I opted for the latter option, because ffi_client.h and this typedef are public API, the other instances in the streams are private member variables. Open to discussion
| fi | ||
| fi | ||
| ;; | ||
| debug-all) |
There was a problem hiding this comment.
are these changes belonging to this clang-tidy PR ?
Or you need to rebase ?
There was a problem hiding this comment.
Intentional: the release-all is now used to build the code and tests for clang-tidy parsing, the debug-all is just to have feature parity on our build options
stephen-derosa
left a comment
There was a problem hiding this comment.
in general this looks good, thanks for doing! I have some questions/comments, nothing major
| submodules: recursive | ||
| fetch-depth: 1 | ||
|
|
||
| - name: Install dependencies |
There was a problem hiding this comment.
holy deps of clang, im surprised there isnt a clanh-tidy ez unstall
There was a problem hiding this comment.
I might be able to clean this up -- standby
|
|
||
| // Listener id registered on FfiClient | ||
| std::int64_t listener_id_{0}; | ||
| std::int32_t listener_id_{0}; |
There was a problem hiding this comment.
why are we reducing size here?
There was a problem hiding this comment.
ffi returns a int32_t?
This PR adds support for
clang-tidystatic analysis within the CI. The list of initial checks can be found in the.clang-tidyfile at the root of the repository.Fixes to
.h/.cppfiles flagged by the analysis were also done.